feat(azdext): Add KeyVaultResolver for extension Key Vault secret resolution#7043
Open
jongio wants to merge 19 commits intoAzure:mainfrom
Open
feat(azdext): Add KeyVaultResolver for extension Key Vault secret resolution#7043jongio wants to merge 19 commits intoAzure:mainfrom
jongio wants to merge 19 commits intoAzure:mainfrom
Conversation
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
from
6efe774 to
808a1fe
Compare
…olution
Add KeyVaultResolver to the Extension SDK for resolving Azure Key Vault
secret references embedded in environment variables. Supports three
reference formats:
- akvs://<subscription>/<vault>/<secret>
- @Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<name>[/<version>])
- @Microsoft.KeyVault(VaultName=...;SecretName=...)
Features:
- Thread-safe per-vault client caching
- Batch resolution via ResolveMap
- Structured error types with ResolveReason classification
- Configurable vault suffix for sovereign clouds
- Helper functions: IsSecretReference, ParseKeyVaultAppReference,
ResolveSecretEnvironment for bulk env var resolution
Also adds supporting functions to pkg/keyvault:
- IsKeyVaultAppReference, IsSecretReference
- ParseKeyVaultAppReference for @Microsoft.KeyVault format
- SecretFromKeyVaultReference for unified resolution
- ResolveSecretEnvironment for bulk env var processing
Evidence: Extensions like azd-exec, azd-app duplicate 100+ lines of KV
resolution logic each. This centralizes the pattern in the SDK.
Related: Azure#6945, Azure#6853
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reverted detect_confirm_apphost.go (had color.MagentaString() without import, causing build failure) and show.go (KV comments belong on improvements branch) to match upstream/main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 'secret' map key in test data triggers gosec G101 (hardcoded credentials) but these are fake akvs:// URIs in test fixtures, not real credentials. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- SSRF: Validate KeyVault SecretUri hostname against Azure vault suffixes - Scoping: Only resolve azd env vars through KV resolver, not os.Environ() - Errors: Use ServiceError reason for non-ResponseError failures - Security: Explicit DisableChallengeResourceVerification: false - Testing: Add 8 tests for @Microsoft.KeyVault reference format - Reliability: Return empty string on resolution failure, not raw reference - Determinism: Sort keys in ResolveMap, collect all errors with errors.Join Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add 'managedhsm' and 'microsoftazure' to cspell dictionary - Suppress gosec G101 false positive on test data string Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
force-pushed
the
feature/ext-sdk-kv-resolver
branch
from
2729231 to
b682e24
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds shared Key Vault secret-reference resolution to the extension SDK / CLI flow, so extensions can receive resolved secret values (rather than raw akvs://... or @Microsoft.KeyVault(...) references) without duplicating parsing + fetch logic.
Changes:
- Add
azdext.KeyVaultResolverwith structured error classification and batch resolution (ResolveMap). - Extend core
pkg/keyvaultwith parsing helpers for@Microsoft.KeyVault(SecretUri=...)and an env-var resolver (ResolveSecretEnvironment). - Integrate env-var resolution into extension invocation (
cmd/extensions.go) and update spelling dictionary entries.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
cli/azd/pkg/keyvault/keyvault.go |
Adds @Microsoft.KeyVault(SecretUri=...) parsing + unified reference resolution + env-var resolution helper. |
cli/azd/pkg/azdext/keyvault_resolver.go |
Introduces the extension-facing KeyVaultResolver API, parsing, and structured error types. |
cli/azd/pkg/azdext/keyvault_resolver_test.go |
Unit tests covering resolver behavior, parsing, and error classification. |
cli/azd/cmd/extensions.go |
Resolves KV references in azd-managed env vars before launching extension processes. |
cli/azd/.vscode/cspell.yaml |
Adds KV-related terms to cspell dictionary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Call ResolveSecretEnvironment unconditionally so akvs:// refs work without AZURE_SUBSCRIPTION_ID being set - Add per-vault client cache (sync.Map) to avoid N client creations when resolving N secrets from the same vault - Preserve original reference in ResolveMap output on failure so callers see all input keys - Tighten IsKeyVaultAppReference to only match SecretUri= variant - Use u.Hostname() for port-safe host validation in SSRF checks - Return ([]string, error) from ResolveSecretEnvironment so callers can handle failures explicitly - Update docs to clarify only SecretUri= format is supported Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@copilot please re-review the latest changes. |
wbreza
reviewed
Mar 17, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
What Looks Good
- Azure KV DNS suffixes are complete — all 5 valid suffixes (public, China, US Gov, Germany deprecated, Managed HSM) match Azure documentation
- Vault name regex is correct — enforces Azure's 3-24 char naming rules accurately
- SSRF protection — host validation against known Azure DNS suffixes prevents arbitrary URL resolution
- Structured error types — KeyVaultResolveError with ResolveReason classification is well-designed
- Per-vault client caching — sync.Map-based getOrCreateClient with LoadOrStore is correct and race-free
- Error collection — ResolveMap and ResolveSecretEnvironment collect all errors via errors.Join
- Reference leak prevention — on failure, env var value is set to empty rather than leaking the raw akvs:// reference
Findings Summary
| Priority | Count |
|---|---|
| Critical | 1 |
| High | 2 |
| Medium | 7 |
| Low | 2 |
| Total | 12 |
Overall Assessment: Comment. Good feature with solid security posture. The inline comments below highlight the sort.Strings bug and areas where additional unit tests would strengthen confidence before merge. Would love to see some more unit tests to validate the PR findings.
|
|
||
| if len(errs) > 0 { | ||
| return result, fmt.Errorf("failed to resolve Key Vault references: %w", errors.Join(errs...)) | ||
| } |
Contributor
There was a problem hiding this comment.
[Critical] sort.Strings(result) breaks env var override semantics
In extensions.go, system env vars from os.Environ() are appended first, then azd vars after -- last-wins semantics for duplicate keys. Sorting the azd vars alphabetically destroys this ordering guarantee. For example, an azd var PATH=/custom intended to override the system PATH could end up in the wrong position after sorting.
Consider removing sort.Strings(result) entirely. If deterministic output is needed for testing, apply it only in tests.
Member
Author
There was a problem hiding this comment.
Fixed — removed sort.Strings(result) entirely. The env var ordering (system first, azd overrides after) is now preserved. Commit: bc6da7e
| } | ||
|
|
||
| // AzureKeyVaultSecret represents a secret stored in an Azure Key Vault. | ||
| // It contains the necessary information to identify and access the secret. |
Contributor
There was a problem hiding this comment.
[High] No tests for ResolveSecretEnvironment or SecretFromKeyVaultReference
Both are new public functions with non-trivial logic (reference dispatch, error collection, env var parsing, fallthrough to unrecognized format error). Key untested paths include: mixed akvs:// and @Microsoft.KeyVault refs, malformed env vars (no = sign), nil kvService passthrough, and unrecognized format fallthrough.
Consider adding table-driven tests with a mock KeyVaultService.
Member
Author
There was a problem hiding this comment.
Fixed — added table-driven tests for both ResolveSecretEnvironment and SecretFromKeyVaultReference covering: mixed akvs:// and @Microsoft.KeyVault refs, malformed env vars (no = sign), nil kvService passthrough, unrecognized format fallthrough, successful resolution, and error collection. Commit: bba1b14
| t.Fatal("expected error for nil context") | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
[High] IsSecretReference test is missing @Microsoft.KeyVault cases
IsSecretReference returns true for both akvs:// and @Microsoft.KeyVault(SecretUri=...), but the test only covers akvs:// variants. No test verifies that the @Microsoft.KeyVault format returns true, nor that the unsupported VaultName/SecretName form returns false.
Consider adding entries like:
@Microsoft.KeyVault(SecretUri=https://v.vault.azure.net/secrets/s)should be true@Microsoft.KeyVault(VaultName=v;SecretName=s)should be false
Member
Author
There was a problem hiding this comment.
Fixed — added test entries for @Microsoft.KeyVault(SecretUri=https://v.vault.azure.net/secrets/s) (expected true) and @Microsoft.KeyVault(VaultName=v;SecretName=s) (expected false). Commit: e81c9a5
| envVars []string, | ||
| defaultSubscriptionId string, | ||
| ) ([]string, error) { | ||
| if kvService == nil { |
Contributor
There was a problem hiding this comment.
[Medium] Non-standard ports pass vault host validation
Host validation uses u.Hostname() (strips port), but VaultURL is built from u.Host (preserves port). A URI like https://myvault.vault.azure.net:9999/secrets/foo passes validation but sends the bearer token to port 9999.
Consider rejecting non-standard ports explicitly, or using u.Hostname() consistently for VaultURL construction.
Member
Author
There was a problem hiding this comment.
Fixed — ParseKeyVaultAppReference now explicitly rejects non-standard ports. Only port 443 or no port is allowed. A URI like https://myvault.vault.azure.net:9999/secrets/foo now returns an error, preventing bearer token leakage to arbitrary ports. Commit: 87b6231
| var errs []error | ||
|
|
||
| for i, envVar := range envVars { | ||
| before, after, ok := strings.Cut(envVar, "=") |
Contributor
There was a problem hiding this comment.
[Medium] Empty vault name accepted when hostname equals a bare suffix
If the SecretUri hostname is exactly a valid suffix (e.g., .vault.azure.net), isValidVaultHost passes because strings.HasSuffix matches. Then vault name extraction on the next line yields an empty string.
Consider checking that dotIdx > 0 after strings.Index(host, '.').
Member
Author
There was a problem hiding this comment.
Fixed — added a dotIdx > 0 check after strings.Index(host, '.'). Bare suffixes like .vault.azure.net now correctly fail with an empty vault name error. Commit: 5704082
|
|
||
| const secretURIPrefix = "SecretUri=" | ||
| if !strings.HasPrefix(inner, secretURIPrefix) { | ||
| return KeyVaultAppReference{}, fmt.Errorf( |
Contributor
There was a problem hiding this comment.
[Medium] Case-sensitive prefix matching may silently skip valid references
Azure App Service handles @Microsoft.KeyVault(...) references case-insensitively. The code uses case-sensitive strings.HasPrefix. Variations like @microsoft.keyvault(SecretUri=...) would be silently ignored -- the raw reference string passes through to the extension process unresolved.
Consider using case-insensitive matching for the wrapper prefix.
Member
Author
There was a problem hiding this comment.
Fixed — IsKeyVaultAppReference and ParseKeyVaultAppReference now use strings.EqualFold for the @Microsoft.KeyVault( prefix matching. Variations like @microsoft.keyvault(SecretUri=...) are now correctly recognized. The URI content itself remains case-sensitive. Commit: a10fb73
| // SecretFromKeyVaultReference resolves a secret reference in either the | ||
| // akvs:// or @Microsoft.KeyVault(SecretUri=...) format. The subscriptionId | ||
| // is required for credential scoping; for @Microsoft.KeyVault references | ||
| // (which lack a subscription), the caller should provide the environment's |
Contributor
There was a problem hiding this comment.
[Medium] Adding a method to an exported interface is a breaking change
Adding SecretFromKeyVaultReference to the KeyVaultService interface breaks all external implementations. The method is only called from ResolveSecretEnvironment (a package-level function). Consider a narrower interface or standalone function to avoid breaking downstream consumers.
Member
There was a problem hiding this comment.
we don't have any external implementations (and shouldn't) for kv service.
Member
Author
There was a problem hiding this comment.
Agreed with @vhvb1989 — KeyVaultService is an internal interface with no external implementations. No change needed here.
|
|
||
| if ref.VaultName != "myvault" { | ||
| t.Errorf("VaultName = %q, want %q", ref.VaultName, "myvault") | ||
| } |
Contributor
There was a problem hiding this comment.
[Medium] Tests use context.Background() throughout (15 occurrences) instead of t.Context() per repo Go 1.26 conventions.
Member
Author
There was a problem hiding this comment.
Fixed — replaced all 15 occurrences of context.Background() with t.Context() in keyvault_resolver_test.go. Commit: 5f7da74
| // | ||
| // akvs://<subscription-id>/<vault-name>/<secret-name> | ||
| // @Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<secret>[/<version>]) | ||
| // |
Contributor
There was a problem hiding this comment.
[Medium] Modern Go: consider slices.Sorted(maps.Keys(refs)) instead of manual key collection + sort.Strings(keys). Same applies to the errors.As call (~line 178) which could use errors.AsType[*azcore.ResponseError](err).
Member
Author
There was a problem hiding this comment.
Fixed — replaced manual key collection + sort.Strings(keys) with slices.Sorted(maps.Keys(refs)) in ResolveMap. Commit: 044aab0
| t.Errorf("ResolveReason(%d).String() = %q, want %q", tt.reason, got, tt.want) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[Low] A few test improvement opportunities:
- The 4 HTTP error classification tests (NotFound, Forbidden, Unauthorized, ServerError) are structurally identical -- a single table-driven test would reduce ~100 lines to ~30
stubSecretGetterignores name/version args, so tests never verify the correct secret name or version is forwarded to GetSecret. A recording stub would catch dispatch bugsTestResolveMap_ErrorCollectsAllFailuresonly has 1 failing ref -- adding 2+ would actually test the collection behavior
Member
Author
There was a problem hiding this comment.
Fixed — consolidated 4 HTTP error classification tests into a single table-driven test, replaced stubSecretGetter with a recording stub that captures name/version args for verification, and added multiple failing refs to the error collection test. Commit: d2dda69
ResolveSecretEnvironment appends system env vars first then azd vars, so last-wins ordering matters for duplicate keys. Sorting alphabetically destroys this override semantics. Addresses review thread 1 (keyvault.go:651).
Azure App Service handles @Microsoft.KeyVault(...) references case-insensitively. Switch from strings.HasPrefix to strings.EqualFold for the prefix and SecretUri= parameter checks so that variations like @microsoft.keyvault(secreturi=...) are not silently skipped. Addresses review thread 6 (keyvault.go:540).
A URI like https://myvault.vault.azure.net:9999/secrets/foo would pass host validation but send the bearer token to an unexpected port. Reject any port other than 443 (or no port) for SSRF protection. Addresses review thread 4 (keyvault.go:617).
If the SecretUri hostname has no subdomain prefix (e.g., 'vault.azure.net' instead of 'myvault.vault.azure.net'), vault name extraction would yield an incorrect or empty value. Add a guard to ensure the extracted vault name is non-empty and distinct from the full host. Addresses review thread 5 (keyvault.go:625).
Replace the manual key collection + sort.Strings pattern with the idiomatic Go 1.26 equivalent. This is cleaner and consistent with the rest of the codebase. Addresses review thread 9 (keyvault_resolver.go:299).
Per Go 1.24+ conventions and repo patterns, use t.Context() to get a test-scoped context that is automatically cancelled when the test ends. Addresses review thread 8 (keyvault_resolver_test.go:548).
The test only covered akvs:// variants. Add cases for the @Microsoft.KeyVault(SecretUri=...) format (true), case-insensitive prefix matching (true), and the unsupported VaultName/SecretName form (false). Addresses review thread 3 (keyvault_resolver_test.go:534).
…llection - Replace 4 structurally identical HTTP error tests with a single table-driven TestResolve_HTTPErrorClassification - Make stubSecretGetter record name/version args and verify dispatch in TestResolve_Success and TestResolve_AppRefWithVersion - Add 3 failing refs (+ 1 plain value) to TestResolveMap_ErrorCollectsAllFailures to verify error collection and partial results Addresses review thread 10 (keyvault_resolver_test.go:766).
…ference Cover the key untested paths: - Mixed akvs:// and @Microsoft.KeyVault refs (both resolved) - Malformed env vars (no = sign) passed through unchanged - nil kvService passthrough (returns input as-is) - Error collection when multiple refs fail - Ordering preserved (no alphabetical sort) - Non-standard port rejection - Port 443 allowed - Empty vault name rejection - Case-insensitive prefix matching - Comprehensive IsSecretReference coverage Addresses review thread 2 (keyvault.go:424).
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Global Key Vault Resolution for azd-Passed Environment Variables
Summary
This PR updates Key Vault handling so resolution is not extension-specific. Any azd-managed environment variable value passed by azd to subprocesses is resolved centrally first; extensions then inherit those resolved values automatically.
Intended Behavior
Supported Reference Formats
akvs://<subscription-id>/<vault-name>/<secret-name>@Microsoft.KeyVault(SecretUri=https://<vault>.vault.azure.net/secrets/<secret>[/<version>])Key Changes
pkg/keyvault:SecretFromKeyVaultReference(...)ResolveSecretEnvironment(...)forKEY=VALUElistspkg/execviaRunnerOptions.EnvironmentResolver.cmd/container.go) so azd env values are resolved before subprocess invocation.Scope Clarification
This PR is about centralized azd env var resolution. Extension behavior is inherited from that central mechanism rather than implemented as extension-only logic.
Validation
go test ./pkg/keyvault ./pkg/execgo test ./cmd -run TestNope